Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(builtin): generate @npm//foo__all_files filegroup that includes all files in the npm package #1600

Merged

Conversation

gregmagolan
Copy link
Collaborator

@gregmagolan gregmagolan commented Feb 4, 2020

Including files with spaces & unicode characters & files that have been filtered out by the included_files attribute. This filegroup cannot be used in runfiles because of Bazel issue bazelbuild/bazel#4327, but it can be used for other purposes such as the srcs input to a pkg_tar for generating a tar of an npm package pulled down with yarn_install or npm_install.

Needed for angular/angular#33927 where pkg_tar is used to make a tar.gz of node_modules/puppeteer and on OSX the downloaded Chrome libs have spaces in their names:

def npm_package_archives():
    """Function to generate pkg_tar definitions for WORKSPACE yarn_install manual_build_file_contents"""
    npm_packages_to_archive = NPM_PACKAGE_ARCHIVES
    result = """load("@bazel_tools//tools/build_defs/pkg:pkg.bzl", "pkg_tar")
"""
    for name in npm_packages_to_archive:
        label_name = _npm_package_archive_label(name)
        last_segment_name = name if name.find("/") == -1 else name.split("/")[-1]
        result += """pkg_tar(
    name = "{label_name}",
    # use //{name}:{last_segment_name}__all_files as this includes **all** of the npm package
    # files including files with spaces and unicode chars which are not valid in Bazel runfiles.
    srcs = ["//{name}:{last_segment_name}__all_files"],
    extension = "tar.gz",
    strip_prefix = "./node_modules/{name}",
    # should not be build unless it is a dependency of another rule
    tags = ["manual"],
)
""".format(name = name, label_name = label_name, last_segment_name = last_segment_name)
    return result

See angular/angular#33927 (comment) for more details.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@buildsize
Copy link

buildsize bot commented Feb 4, 2020

File name Previous Size New Size Change
release.tar.gz 963.05 KB 963.94 KB 912 bytes (0%)

@gregmagolan gregmagolan changed the title feat(builtin): generate @npm//foo__raw_files filegroup that includes all files in the npm package feat(builtin): generate @npm//foo__all_files filegroup that includes all files in the npm package Feb 4, 2020
…all files in the npm package

Including files with spaces & unicode characters & files that have been filtered out by the `included_files` attribute. This filegroup cannot be used in runfiles because of Bazel issue bazelbuild/bazel#4327, but it can be used for other purposes such as the srcs input to a pkg_tar for generating a tar of an npm package pulled down with yarn_install or npm_install.
@@ -13,7 +13,6 @@ filegroup(
"//:node_modules/rxjs/InnerSubscriber.d.ts",
"//:node_modules/rxjs/InnerSubscriber.js",
"//:node_modules/rxjs/InnerSubscriber.js.map",
"//:node_modules/rxjs/LICENSE.txt",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's technically a breaking change if users referred to one of these target directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. No breaking change here. This is just testing that the include_files works are intended and that any files that aren't included still end up the __all_files target.

// of package files that end with `.umd.js`, `.ngfactory.js` and `.ngsummary.js`.
// TODO(gmagolan): add UMD & AMD scripts to scripts even if not an APF package _but_ only if they
// are named?
const namedSources = isNgApfPackage(pkg) ?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so many changes in this file - I'm assuming it's just a refactoring moving lines around, and not this many lines edited?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Some refactoring here.

@gregmagolan gregmagolan merged commit 8d77827 into bazel-contrib:master Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants